[2단계 - 상세 정보 & UI/UX 개선하기] 윤생(이윤성) 미션 제출합니다. #92
Conversation
There was a problem hiding this comment.
안녕하세요 윤생~ 티케입니다.
미션을 정말 빠르게 해주셨는데, 리뷰가 두번씩이나 많이 늦어서 정말 죄송하다는 말로는 부족하지만, 죄송한 마음을 전합니다.. 많이 속상하셨을 마음도 이해가 가네요. 😢
Step1에서도 말했지만, 코드가 깔끔하고 함수분리가 대부분 잘 되어있어서, 크게 고쳐야하는 부분은 없었고, 리팩토링을 하면 좋을 부분을 위주로 코멘트를 드렸습니다. 드디어 방학이네요! level1의 미션이 마무리가 저때문에 늦었지만(다시한번 죄송합니다ㅠㅠ), 방학동안 그동안 배웠던 부분을 정리하고 리프레시하고 푹 쉬는 시간이 되길 바랍니다 :) 개인적으로 공부를 해야한다면 리액트보다는 타입스크립트를 한번 살펴보는 시간이 되면 좋을 것 같아요!
전반적인 코드를 다시 쭉 살펴보면서, file changes에 잡히지 않아서 코멘트를 드리지 못한 부분을 포함해서 피드백을 드렸고(아래에 있어요), 소소한 피드백을 남겼습니다. 미션은 바로 approve하려고 합니다. 리뷰가 늦었기 때문에, 주말까지 병합을 하지 않을테니 질문이 있다면 남겨주세요 아.. 병합을 안하면 메세지 안가네요... 댓글 그냥 남겨주시면 이메일로 알림 와서 틈틈히 답변드리겠습니다. DM으로 주셔도 좋구요 :)
피드백
MovieListPageComponent.js
const movieList = JSON.parse(this.getAttribute("data-movie-list"));에서 movieList가 없으면 어떻게 되나요?
MovieListComponent.js
- 여전히 dataset에 모든 정보가 담기는 건 아쉽네요. 이 부분 뿐만 아니라, 프로젝트 전반적으로 data-* attributes를 많이 활용을 하셨는데, 그것을 만능처럼 쓰는 부분이 있는 것 같다는 생각이 들었어요.
MovieComponent
import StarEmptyImg from "../../../templates/star_empty.png";안쓰여용
ErrorComponent
- 네이밍은 에러 컴포넌트이지만, 실제로는
ErrorPage에 가까운 것 같아요.
abstracts > types.ts
- 파일의 위치가 어색해요. 추상화 계층을 다루는 곳이라고 생각해서
custom Component를 담아둔것은 이해가 갔지만.types.ts에 있는 건 api응답에 가깝지 않을까 하는 생각이 드네요.models나api하위가 더 잘어울릴 것 같다는 생각도 들고.. 더 좋은 아이디어가 있겠지만 현재의 위치는 어색해요.
질문/의견
- css에 대해서 의견을 주었는데, 저도 처음에는 생각나는 대로 작성하는데, 나중에 PR을 올릴때는 알파벳 순으로 수정하는 편이에요. 절대 불변이기 때문에(알파벳순이 갑자기 abdc로 바뀌진 않으니..) 다른 사람들과 컨벤션을 논의할 시간을 줄여주니깐요..! 하지만 그럼 어떤 스타일인지 인지가 빠르게 안될수도 있기 때문에 https://rhodesmill.org/brandon/2011/concentric-css/ 이걸 참고해봐도 좋을 것 같아요!
| @@ -188,3 +188,23 @@ describe("네트워크 에러 확인", () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("영화 상세 정보(모달창)", () => { | |||
There was a problem hiding this comment.
PR 메세지에 써두긴 했지만, 별점에 대한 테스트도 추후에 보완이 되면 좋을 것 같아요 :)
| @@ -49,13 +63,17 @@ export default class AppComponent extends CustomComponent { | |||
| } | |||
|
|
|||
| changeButtonDisplayByPage() { | |||
| if (this.#totalPage <= this.#nextPage) { | |||
| if (this.isEndOfPage()) { | |||
| case ACTION.DETAIL: | ||
| const movieId = e.target.dataset.movieId; | ||
| getRequest( | ||
| `${REQUEST_URL}/movie/${movieId}?api_key=${API_KEY}&language=ko-KR` | ||
| ) | ||
| .then((res) => { | ||
| const modal = document.querySelector("modal-component"); | ||
| modal.setAttribute("data-item", JSON.stringify(res)); | ||
|
|
||
| modal.style.display = "flex"; | ||
| document.body.style.overflow = "hidden"; | ||
| }) | ||
| .catch(() => { | ||
| alert(ERROR_MESSAGE); | ||
| }); | ||
| break; |
There was a problem hiding this comment.
이부분은 다른 부분과 달리 raw한 느낌이 드는데, 다른 함수로 빼는 방법은 어려웠을까요?
혹은 click이라는 이벤트를 너무 한번에 다 묶으려다보니 성급한 추상화로 인해서 일관성 맞추기가 어려웠을 것 같기도 하네요.
저번에도 비슷한 피드백을 드린 것 같은데 "도메인 관련 로직을 최대한 상위로 빼, 재사용이 가능한 컴포넌트들에게 로직의 책임을 덜 관여하게 하고 싶었음을 의도하고 싶었습니다."의 단점이 드러나는 부분이 아닐까? 싶기도 합니다.
오랜만에 코드를 봤을 때 이 click 이벤트 핸들러는 누구의 이벤트일까? 라는 고민을 많이 하게 만들었던 것 같기도 하네용.
| if (!this.#scrollThrottleId) { | ||
| this.#scrollThrottleId = setTimeout(() => { | ||
| if ( | ||
| this.getBoundingClientRect().bottom - window.innerHeight < | ||
| SCROLL_INVOKE_GAP | ||
| ) { | ||
| this.#$movieList.appendNewPage(); | ||
| this.getMovieData(this.#actionType); | ||
| } | ||
| this.#scrollThrottleId = null; | ||
| }, 1000); |
There was a problem hiding this comment.
스크롤 이벤트 처리를 this.getBoundingClientRect().bottom - window.innerHeight < SCROLL_INVOKE_GAP(app이 viewport로 부터 얼마나 떨어져 있는지) 확인하므로, 스크롤 이벤트가 발생할 때 쓰로틀링을 걸어주지 않으면 중복되어 호출이 일어나더라구요..!
|
|
||
| toggleUpScrollButton() { | ||
| const header = document.querySelector("app-header"); | ||
| const upScrollBtn = document.querySelector("up-scroll-button"); |
There was a problem hiding this comment.
이런 UX 너무 좋습니다 💯
이 컴포넌트 너무 크다는 생각이 드네용,,!
|
|
||
| import StarFilledImg from "../../templates/star_filled.png"; | ||
| import StarEmptyImg from "../../templates/star_empty.png"; | ||
| import { OVERVIEW_EMPTY } from "../constants/constants"; | ||
| export default class ModalComponent extends CustomComponent { |
There was a problem hiding this comment.
ModalComponent라는 네이밍과 다르게, 그리고 components의 바로 하위에 있는 컴포넌트 치고는 너무 도메인 종속적이라서 아쉽네요..!
There was a problem hiding this comment.
전체적으로 코드를 다시 보다가 생각난 피드백을 여기에 적을게요~
Index.js외에도 다른 컴포넌트에서도 웹 컴포넌트import에 대한 주석은 있었으면 좋겠습니다. 사람들이 무조건index.js를 통해서 다른 파일들을 본다는 보장이 없으니 안쓰이는 파일로 오해할수도 있을 것 같아요import transformMovieItemsType from "../util/MovieList";이 파일은 더이상 쓰이지 않네요. 파일과 Import 영역 모두 삭제해주세요~changeButtonDisplayByPage()에 대해서 무한스크롤로 구현을 했지만, 사용자에게 중간중간 '더보기' 버튼이 노출이 되는 것 같아요. 보통의 서비스에서 무한스크롤 UX가 어떻게 되는지 찾아봐서 참고하면 어떨까요? (혹은intersection observer의element가 어떤 UI를 가지는 지 찾아봐도 좋구요)handleEvent가 너무 길어서, 이벤트별로 분리를 해보는 건 어떨까요?
안녕하세요 티케~! 주말 잘 보내셨나요?
2단계 PR 제출합니다!
2단계 주요 기능 및 요구사항은 다음과 같습니다. 추가로 1단계에서 생각한 UI & UX를 개선해보려고 고민을 해봤는데, 많이 개선하지는 못한 것 같아요.
기능 요구사항
무한스크롤
반응형 디자인
영화 상세 조회 (모달창)
영화 별점 매기기 (로컬 스토리지에 저장)
아쉬운 점
궁금한 점
감사합니다~!
Movie List 주소 : 🎬 Movie List